-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop temporaries created in a condition, even if it's a let chain #102998
Conversation
by the end of the condition's execution
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
Hmmm not sure what that ICE in CI is about, I can’t reproduce it locally. I’ll have to take a deeper look later. |
This comment has been minimized.
This comment has been minimized.
Does this also fix #100276? |
I am not qualified to approve this PR i think 😂 r? compiler also returning to author to fix the bug @rustbot author |
I ran into an issue recently that only showed up in CI. I was able to repro it locally by runner the tests in docker: https://rustc-dev-guide.rust-lang.org/tests/docker.html Maybe that would help here? |
Hmm unfortunately not, it seems to be a separate bug. (Though now that I'm looking at it, I think I actually might know the problem there - I'll try to open a separate PR, hopefully soon-ish). |
Ah, I was actually able to fix it based on @compiler-errors comment, but thanks for the pointer! I'm sure it'll come in handy in the future. With the latest commits I think this should be ready for review, so |
The behavior change seems correct to me. (No comment on the implementation.) |
The implementation seems reasonable to me, so this PR seems good to go. @bors r+ |
if let Some(_d) = self.option_loud_drop(18) // 5 | ||
&& let Some(_e) = self.option_loud_drop(17) // 4 | ||
&& self.option_loud_drop(14).is_some() // 1 | ||
&& self.option_loud_drop(15).is_some() { // 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The drop order is a bit surprising. IIUC, we start by dropping the first regular conditions in reverse source order, then other regular conditions in forward source order, then the block, then the let conditions.
Why the forward/reverse mix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's definitely a quirk. It's pretty much because for that condition we go from an AST like
// Fake notation - the numbers correspond to the argument to `option_loud_drop`
Binary(And, Binary(And, Binary(And, Let(18), Let(17)), MethodCall(14)), MethodCall(15)
to hir like
Binary(And, Binary(And, Binary(And, Let(18), Let(17)), DropTemps(MethodCall(14))), DropTemps(MethodCall(15)))
Ideally we could group
self.option_loud_drop(14).is_some() // 1
&& self.option_loud_drop(15).is_some() // 2
into the same DropTemps
and then they would drop in reverse source order, but due to the nesting, I can't see a way to wrap them in the same DropTemps
without the let conditions being lumped in.
If the regular conditions come first, like
self.option_loud_drop(14).is_some()
&& self.option_loud_drop(15).is_some()
&& let Some(_d) = self.option_loud_drop(18)
&& let Some(_e) = self.option_loud_drop(17)
we can (and do) wrap them in the same DropTemps
, and so they drop in reverse source order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR there is also pretty weird behaviour for vanilla non-let if chains. Drop behaviour is currently like:
if self.option_loud_drop(5).is_some()
&& self.option_loud_drop(1).is_some()
&& self.option_loud_drop(2).is_some()
&& self.option_loud_drop(3).is_some()
&& self.option_loud_drop(4).is_some() {
self.print(6);
}
Same goes if you put the expression into a let _= ...
, or replace the &&
with a ||
(while changing the is_some to is_none so that all partial exprs get evaluated). So second sub-expression is first, then it goes in normal direction, until the end. Then comes the first sub-expression. Non-short-circuiting & and | drop more consistently in inverse order (5,4,3,2,1). I'm not sure how to resolve this, and how to integrate this with let chains. There is the danger of furthering technical debt on one hand, on the other hand there is the danger of adding exceptions.
A bit unrelated: Generally I think that dropping the temporaries of the if let expression after the else
block is entered was a mistake. Ideally it should be changed to always pre-drop, like what if is doing. I think let chains give us a good opportunity to switch to the new behaviour, at least for let chains, and I'm very happy that currently this is what this PR seems to be doing: drop all temporaries, from let or from an expression, before the else
block is entered. Just as a positive feedback :).
Rollup of 6 pull requests Successful merges: - rust-lang#102773 (Use semaphores for thread parking on Apple platforms) - rust-lang#102884 (resolve: Some cleanup, asserts and tests for lifetime ribs) - rust-lang#102954 (Add missing checks for `doc(cfg_hide(...))`) - rust-lang#102998 (Drop temporaries created in a condition, even if it's a let chain) - rust-lang#103003 (Fix `suggest_floating_point_literal` ICE) - rust-lang#103041 (Update cargo) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #100513.
During the lowering from AST to HIR we wrap expressions acting as conditions in a
DropTemps
expression so that any temporaries created in the condition are dropped after the condition is executed. Effectively this means we transforminto (roughly)
so that if we create any temporaries, they're lifted into the new scope surrounding the condition, so for example something along the lines of
Before this PR, if the condition contained any let expressions we would not introduce that new scope, instead leaving the condition alone. This meant that in a let-chain like
the temporary created for
get_drop("first")
would be lifted into the surrounding block, which caused it to be dropped after the execution of the entireif
expression.After this PR, we wrap everything but the
let
expression in terminating scopes. The upside to this solution is that it's minimally invasive, but the downside is that in the worst case, an expression withlet
exprs interspersed likegets multiple new scopes, roughly
so instead of all of the temporaries being dropped at the end of the entire condition, they will be dropped right after they're evaluated (before the subsequent
let
expr). So while I'd say the drop behavior around let-chains is less surprising after this PR, it still might not exactly match what people might expect.For tests, I've just extended the drop order tests added in #100526. I'm not sure if that's the best way to go about it, though, so suggestions are welcome.